Add integration test for mw::com add-on configurations - #1030
Add integration test for mw::com add-on configurations#1030SebSparrowHawk wants to merge 2 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
a665a4a to
f298cbf
Compare
|
Please fix #1030 (review) |
2994dfd to
8480d1c
Compare
LittleHuba
left a comment
LittleHuba
left a comment
There was a problem hiding this comment.
In general this looks good.
I'm finding the test somewhat too simple.
What you currently do is just assign a different identifier to the same service interface.
But the reconfiguration is way more powerful.
What I would like to see are additional tests that:
- Add a previously unknown service interface with deployment
- Do something that would cause problems (like reconfiguring a previously known service instance where we already have a Skeleton/Proxy for
- ...
| ":common_resources", | ||
| ":test_constants", | ||
| "//score/mw/com", | ||
| "//score/mw/com/impl:instance_specifier", |
There was a problem hiding this comment.
Please don't depend on impl in integration tests. This adds technical debt.
There was a problem hiding this comment.
Will remove it
| ":common_resources", | ||
| ":test_constants", | ||
| "//score/mw/com", | ||
| "//score/mw/com/impl:instance_specifier", |
|
|
||
| // 2nd step: Load add-on configuration and merge into existing configuration | ||
| const auto service_instance_manifest_path = ParseServiceInstanceManifest(argc, argv); | ||
| const auto add_on_load_result = score::mw::com::runtime::InitializeRuntimeAddonConfiguration( |
There was a problem hiding this comment.
Late to the party...
That name is a mouth full. I'd love to have something easier. E.g. score::mw::com::runtime::Reconfigure()
Why was that name chosen?
There was a problem hiding this comment.
I'm open to a different shorter name. But I'm not sure about Reconfigure(), because it sounds like you could change the existing configuration, while we are only extending it.
About the name's origin: That was chosen by the requesters but I think we can still change that.
There was a problem hiding this comment.
Changed to AddConfiguration(...)
This integration tests ensures that after merging two configurations, an existing instance identifer is still valid and can be used for communication. Also new service instances as defined in the add-on configuration can be created.
8480d1c to
a132622
Compare
3 participants
This integration tests ensures that after merging two configurations, an existing instance identifer is still valid and can be used for communication. Also new service instances as defined in the add-on configuration can be created.